Rework messages serialization #352

Shatur · 2024-11-13T01:26:50Z

The performance is a bit slower, but we getting a free byte per changed entity and several bytes per message.
For example, in our statistic test we using only 16 bytes for a test message instead of 33.

While writing the documentation, I decided to rename InitMessage to ChangeMessage and UpdateMessage to MutateMessage (and related types). This change helps make it clear that MutateMessage stores only component mutations. In contrast, ChangeMessage stores any type of change and may even include mutations if there is an insertion or removal. I considered including the rename into a separate PR, but decided to keep it here since the messages got completely reworked.

I would recommend to start reading from change_message and mutate_message modules, they contain a lot of internal documentation and description about how the new approach works.

I also switched to plain Vec for serialization since we no longer need Cursor for it
and Writer::write_all is much slower then Vec::extend_from_slice.

See comments in the code for details. I also switched to plain `Vec` for serialization since we no longer need `Cursor` for it and `Writer::write_all` is much slower then `Vec::extend_from_slice`.

codecov · 2024-11-13T01:59:23Z

Codecov Report

Attention: Patch coverage is 94.68268% with 31 lines in your changes missing coverage. Please review.

Project coverage is 89.71%. Comparing base (df44894) to head (d317805).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/client.rs	91.87%	13 Missing ⚠️
src/core/replication/replicated_clients.rs	87.50%	5 Missing ⚠️
src/core/event_registry/server_event.rs	69.23%	4 Missing ⚠️
src/server.rs	96.15%	4 Missing ⚠️
src/server/replication_messages/serialized_data.rs	91.17%	3 Missing ⚠️
src/server/replication_messages/change_message.rs	98.36%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #352      +/-   ##
==========================================
+ Coverage   89.32%   89.71%   +0.39%     
==========================================
  Files          41       44       +3     
  Lines        2388     2450      +62     
==========================================
+ Hits         2133     2198      +65     
+ Misses        255      252       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

"Arrays" describes it better since the tick is technically also part of the header.

Check most unlikely conditions first.

Just read until the end of the cursor. Saves us a single byte.

I planning to encode other things inside it. Also this name fits better in general.

UkoeHB

Overall looks good. I think we tried a similar approach with ranges a while back but scrapped it due to perf.

UkoeHB · 2024-11-24T21:19:09Z

CHANGELOG.md

+- Rename `ReplicationChannel::Init` into `ReplicationChannel::Changes`.
+- Rename `ReplicationChannel::Update` into `ReplicationChannel::Mutations`.


Suggested change

- Rename `ReplicationChannel::Init` into `ReplicationChannel::Changes`.

- Rename `ReplicationChannel::Update` into `ReplicationChannel::Mutations`.

- Rename `ReplicationChannel::Init` into `ReplicationChannel::Update`.

- Rename `ReplicationChannel::Update` into `ReplicationChannel::Mutations`.

What do you think about this instead? It would disambiguate the message type from the internal subsection called 'changes'. Changes also seems slightly off to me for the message name.

If you agree, let's change it in a follow-up PR.

src/client.rs

src/server/replication_messages/mutate_message.rs

src/server.rs

UkoeHB · 2024-11-25T02:53:29Z

src/server.rs

        for ((message, _), client) in messages.iter_mut().zip(replicated_clients.iter_mut()) {
            client.remove_despawned(entity);
-            message.write_entity(&mut shared_bytes, entity)?;
+            message.add_despawn(entity_range.clone());
        }
    }


Hmm do we need to filter out clients that don't have visibility of the despawned entity? Despawn messages could be considered an information leak.

Noticed this as well while worked on this PR! Maybe in a follow-up since it were like this before?

src/server.rs

UkoeHB · 2024-11-25T03:07:40Z

src/server.rs

+        if !mutate_message.is_empty() {
+            let server_tick = write_tick_cached(&mut server_tick_range, serialized, server_tick)?;


Suggested change

if !mutate_message.is_empty() {

let server_tick = write_tick_cached(&mut server_tick_range, serialized, server_tick)?;

if !mutate_message.is_empty() {

debug_assert!(change_message.is_empty());

let server_tick = write_tick_cached(&mut server_tick_range, serialized, server_tick)?;

Looks like this one was missed

Isn't this one related to the change message not consuming the whole mutate message? I.e. it's possible for mutate message to be sent with change message.

Ok seems that way. For some reason I thought if we had a 'Changes' message then there wouldn't be a 'Mutation' message.

Co-authored-by: UkoeHB <[email protected]>

Shatur

Thanks! Applied or answered all suggestions except https://github.com/projectharmonia/bevy_replicon/pull/352/files#r1855691304, will back to it tomorrow.

Shatur · 2024-11-25T16:21:02Z

CHANGELOG.md

+- Rename `ReplicationChannel::Init` into `ReplicationChannel::Changes`.
+- Rename `ReplicationChannel::Update` into `ReplicationChannel::Mutations`.


src/client.rs

Shatur · 2024-11-25T16:25:21Z

src/client.rs

+    let flags = ChangeMessageFlags::from_bits_retain(cursor.read_fixedint()?);
+    debug_assert!(!flags.is_empty(), "message can't be empty");
+
+    let message_tick = DefaultOptions::new().deserialize_from(&mut cursor)?;


You are right, will revert this part and add a comment.

Perhaps you could use one of the ChangeMessageFlags

Sounds good, but I will leave it for a follow-up.

src/client.rs

Shatur · 2024-11-25T23:28:39Z

src/client.rs

+        let len = apply_dyn_array(&mut Cursor::new(&*mutate.message), |cursor| {
+            apply_mutations(world, params, cursor, mutate.message_tick)
+        });
+
+        match len {


I would prefer to keep it as is. I usually don't write *_result or *_option. In Rust it's common to shadow the name if it's the same thing, but with a different type.

Shatur · 2024-11-25T23:29:03Z

src/client.rs

+    if let Some(mut history) = client_entity.get_mut::<ConfirmHistory>() {
+        history.set_last_tick(message_tick);
+    } else {
+        commands
+            .entity(client_entity.id())
+            .insert(ConfirmHistory::new(message_tick));


Already did it, but in the history rework PR 🙂 Let's keep it as is for this PR to avoid resolving conflicts.

Shatur · 2024-11-25T23:32:20Z

src/server.rs

        for ((message, _), client) in messages.iter_mut().zip(replicated_clients.iter_mut()) {
            client.remove_despawned(entity);
-            message.write_entity(&mut shared_bytes, entity)?;
+            message.add_despawn(entity_range.clone());
        }
    }


Noticed this as well while worked on this PR! Maybe in a follow-up since it were like this before?

src/server.rs

src/server/replication_messages/mutate_message.rs

Reverse the condition and remove else to reuse a few lines.

Shatur

@UkoeHB done!

src/client.rs

src/server.rs

src/server/replication_messages/mutate_message.rs

Optimize replication message packing

b317a6d

See comments in the code for details. I also switched to plain `Vec` for serialization since we no longer need `Cursor` for it and `Writer::write_all` is much slower then `Vec::extend_from_slice`.

Shatur requested a review from UkoeHB November 13, 2024 01:26

Shatur linked an issue Nov 13, 2024 that may be closed by this pull request

Init message header #186

Closed

Shatur changed the title ~~Optimize replication message packing~~ Optimize replication message sizes Nov 13, 2024

Fix doc tests

396d91b

Shatur changed the title ~~Optimize replication message sizes~~ Optimize replication message size Nov 13, 2024

Shatur added 14 commits November 15, 2024 19:30

Add a test for many entities

c1bb570

Replace many entities test with many components

2f727b7

Merge remote-tracking branch 'origin/master' into ser-de-rework

098e18d

Rename InitMessageHeader into InitMessageArrays

4fe95b3

"Arrays" describes it better since the tick is technically also part of the header.

Swap order of InitMessage::is_empty checks

972ab21

Check most unlikely conditions first.

Use "mutation" instead of "change" for update message for clarity

44270cc

Don't serialize size for last array inside init message

b40cab4

Just read until the end of the cursor. Saves us a single byte.

Rename UpdateMessage into MutateMessage

a148ab1

Rename InitMessage into ChangeMessage

e443c8b

Rename mutate_timeout into mutations_timeout

05336f8

Improve trace message

1fcee8b

Update docs

751e25a

Serialize array len for nested arrays of change message

4fb17d2

Split apply_array to avoid having confusing bool arg

6751dc2

Shatur changed the title ~~Optimize replication message size~~ Rework messages serialization Nov 17, 2024

Shatur added 3 commits November 17, 2024 02:42

Add a test for removal with despawn

713f645

Assert if the message is empty

6558824

Always include size for mappings

a37d3ca

Shatur mentioned this pull request Nov 17, 2024

Rework DeferredEntity creation #356

Merged

Shatur added 4 commits November 18, 2024 21:37

Rename InitMessageArrays into InitMessageFlags

9f204f8

I planning to encode other things inside it. Also this name fits better in general.

Print the number of mutate messages sent

397a18c

Merge remote-tracking branch 'origin/master' into ser-de-rework

b866310

Merge branch 'master' into ser-de-rework

9eb97cb

Shatur added 2 commits November 21, 2024 01:00

Merge branch 'master' into ser-de-rework

6c1c60f

Merge branch 'master' into ser-de-rework

024829f

UkoeHB requested changes Nov 25, 2024

View reviewed changes

Apply doc suggestions from code review [skip ci]

07be709

Co-authored-by: UkoeHB <[email protected]>

Shatur commented Nov 25, 2024

View reviewed changes

Shatur added 8 commits November 26, 2024 01:37

Do not use varints for acks

50df358

Don't use varint encoding for ticks

09a6340

Use guarded return

75683ed

Refactor take_mutations

ccb1848

Add more assertions

949a060

Use a single function to apply array

42067db

Bring back can_pack

2ad7a4a

Refactor the packing logic

98c7404

Reverse the condition and remove else to reuse a few lines.

Shatur commented Nov 26, 2024

View reviewed changes

src/client.rs Outdated Show resolved Hide resolved

src/server.rs Show resolved Hide resolved

src/server/replication_messages/mutate_message.rs Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/master' into ser-de-rework

d317805

UkoeHB approved these changes Nov 26, 2024

View reviewed changes

Shatur merged commit 0c7f059 into master Nov 26, 2024
7 checks passed

Shatur deleted the ser-de-rework branch November 26, 2024 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework messages serialization #352

Rework messages serialization #352

Shatur commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading

UkoeHB left a comment

UkoeHB Nov 24, 2024

Shatur Nov 25, 2024

UkoeHB Nov 25, 2024

Shatur Nov 25, 2024

UkoeHB Nov 25, 2024

UkoeHB Nov 26, 2024

Shatur Nov 26, 2024

UkoeHB Nov 26, 2024

Shatur left a comment

Shatur Nov 25, 2024

Shatur Nov 25, 2024

Shatur Nov 25, 2024

Shatur Nov 25, 2024

Shatur Nov 25, 2024

Shatur left a comment

		- Rename `ReplicationChannel::Init` into `ReplicationChannel::Changes`.
		- Rename `ReplicationChannel::Update` into `ReplicationChannel::Mutations`.

		if !mutate_message.is_empty() {
		let server_tick = write_tick_cached(&mut server_tick_range, serialized, server_tick)?;

Rework messages serialization #352

Rework messages serialization #352

Conversation

Shatur commented Nov 13, 2024 • edited Loading

codecov bot commented Nov 13, 2024 • edited Loading

Codecov Report

UkoeHB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shatur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shatur left a comment

Choose a reason for hiding this comment

Shatur commented Nov 13, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading